Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

asset_path respects SCRIPT_NAME. #9646

Closed
wants to merge 2 commits into from

Conversation

senny
Copy link
Member

@senny senny commented Mar 10, 2013

Closes #2992

The code on 3-2-stable is very different but It should be an easy fix too.

@senny
Copy link
Member Author

senny commented Mar 10, 2013

@steveklabnik @guilleiguaran can you take a look?

@@ -133,6 +133,8 @@ def asset_path(source, options = {})
end

relative_url_root = defined?(config.relative_url_root) && config.relative_url_root
request = self.request if respond_to?(:request)
relative_url_root ||= request.script_name if request

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if respond_to?(:request) should be enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it returns nil for ActionMailer, for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should keep it the way it is then. If this PR makes it in, I'd like to perform a quick cleanup/refactoring on the that helper code.

@josevalim
Copy link
Contributor

I am afraid this will cause unintended side effects. If you have a mountable engine, the script_name will also be set but in this case, asset_path still should not consider script path because the entity responsible for delivering assets is in the main application (sprockets in this case).

So I think we should create a mountable engine and ensure we can still access its assets after this patch. Things can get even trickier, because you have can have a mountable engine with Passenger's script_name and in this case we should use only the one coming from passenger. Here are a couple places to look at:

@senny
Copy link
Member Author

senny commented Mar 10, 2013

As far as I can tell AC still has a relative_url_root option but I think this won't fix the problem. If I interpret this test correctly: https://github.com/rails/rails/blob/master/actionpack/test/template/asset_tag_helper_test.rb#L558-L573
That option is already considered when computing the asset path and it won't be set when you change the rack mount point.

@pixeltrix
Copy link
Contributor

@senny you're meant to set config.relative_url_root to the same as the mount point for the application. IIRC, the original intention for Rails 3.0 was to deprecate it and the environment variable RAILS_RELATIVE_URL_ROOT and use SCRIPT_NAME exclusively. However it was undeprecated because of the need to be able get the relative url root outside of a request context like precompiling assets.

I think we need to have a CF chat about the various options and come up with a plan for sorting this out.

@senny
Copy link
Member Author

senny commented Mar 10, 2013

@pixeltrix thanks for clarifying. I put my work on hold and catch you on CF.

@senny
Copy link
Member Author

senny commented Mar 15, 2013

I'm closing this one as this is not a viable solution. I'm working on a different approach.

@senny senny closed this Mar 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asset_path ignores base URI
4 participants